Repository navigation
Share unchanged models in update_table_metadata instead of deep-copying - #4303
Open
ASRodrigo1 wants to merge 1 commit into
Open
ASRodrigo1 wants to merge 1 commit into
ASRodrigo1 wants to merge 1 commit into
Conversation
update_table_metadata ended in model_copy(deep=True), so every call copied the whole table history (snapshots, logs, schemas) and its cost grew with the number of snapshots the table keeps. Transaction.table_metadata calls it on every read and catalogs call it once more on commit, which made one append to a table with 10,000 snapshots take seconds. Copy only the list and dict fields of the metadata and share the frozen models they hold, like TableMetadata.Builder in the Java implementation. The base metadata stays isolated from a list append or dict assignment on the result. Closes apache#4294 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #4294
Rationale for this change
The problem
update_table_metadataends innew_metadata.model_copy(deep=True). Every call therefore copies the entire table history: each snapshot with its summary, each snapshot log and metadata log entry, and each schema, partition spec and sort order. The cost of one call grows linearly with the number of snapshots the table keeps, regardless of how small the update is.The function sits on the hot path of every write:
Transaction.table_metadatacalls it on every read. Oneappendreads the property 22 times.Catalog._update_and_stage_tablecalls it once more on every commit. It is used by the SQL, Hive and Glue catalogs.Each commit also adds a snapshot, so a table that receives frequent small commits pays a little more on each one. #4294 calls this quadratic in total.
How we ran into it
On a production table with a few thousand snapshots retained, a single
fast_appendcommit took close to 10 seconds. Profiling the commit with cProfile and py-spy attributed 71% of its time toupdate_table_metadata→model_copy(deep=True)→copy.deepcopy, with about 4 milliondeepcopycalls per commit.The cost tracks retention: in a reproduction with a larger history, the same commit took 18 s with 10,000 snapshots and 68 s with 40,000. A writer that commits frequently cannot keep up at that depth.
Why the deep copy exists, and why a shallow container copy is enough
The deep copy was introduced in #182 so that the metadata returned by
update_table_metadatacannot change the base metadata.test_metadata_isolation_from_illegal_updatespins that guarantee. It appends tonew_metadata.partition_specsand asserts thatbase_metadata.partition_specsis unchanged. As the test's own comment notes, Pydantic'sfrozenconfig cannot prevent mutations such as a list append.That guarantee only requires the list and dict fields of
TableMetadatato be distinct objects. The models they hold (Snapshot,Schema,PartitionSpec,SortOrder,SnapshotRef, the log entries) are themselves frozen, so they can be shared safely.This is also how the Java implementation works.
TableMetadata.Buildercopies each collection (Lists.newArrayList(base.snapshots()),Maps.newHashMap(base.refs), and so on) and shares the immutable elements.The handlers behind
_apply_table_updatewere already required not to mutate their input in place: the first handler receives the base metadata itself, and the isolation test checks that the base is unchanged after legal updates. We audited all 23 handlers onmainand found that each one builds new collections or returns its input unchanged. This change adds no new requirement on them.The change
The final
model_copy(deep=True)is replaced by_copy_containers, which copies every list and dict field and shares everything else:There is no cache and no change to the API, and the
enforce_validationpath is untouched. Fixing the function rather than its callers makes every path cheap: reads with or without staged updates, and the commit inside each catalog.Relation to earlier work
table_metadatainsnapshot.pyTransaction.table_metadatabetween readsCreateTableTransaction,set_propertiesbefore an append), and the catalog commitupdate_table_metadataitselfThis PR does not conflict with #4302. With both applied, #4302 adds a small further gain (table below).
Measurements
One append of one row to a table registered in a
SqlCatalogon SQLite, with a local warehouse and no network. Each time is the median of 3 appends. To set the depth, the metadata was rewritten to keep N snapshots that point at the table's real manifest list. All four variants ran on the same machine against the same code:mainrestores the deep copy, and#4302applies its change toTransaction.table_metadata.mainupdate_table_metadataalone, adding one snapshot (median of 5):What remains at 40,000 snapshots is mostly reading and writing the 26 MB metadata file, which no change to this function can avoid.
Are these changes tested?
Yes.
test_update_metadata_shares_unchanged_modelschecks three things after an update: the list and dict fields of the result are new objects, the snapshots and schemas are the same objects as in the base, and the base properties are unchanged. It fails onmainand passes with this change.test_metadata_isolation_from_illegal_updatespasses unchanged.pytest tests/ -m unit --ignore=tests/integrationon Python 3.13: 4,521 passed, 5 skipped, 4 failed. The 4 failures are the Kerberos tests intests/catalog/test_hive.py, which fail withNo module named 'puresasl'because thehive-kerberosextra was not installed. They fail the same way onmain(1c800eb) without this change.ruff checkandruff formatare clean. mypy reports no new errors in the changed module.Are there any user-facing changes?
The public API does not change, and commits produce the same metadata as before.
One behavior changes. The models inside the returned metadata are now shared with the base metadata, so mutating one of them in place also changes the base. An example is writing into a snapshot's summary with
metadata.snapshots[0].summary[...] = .... These models are frozen and are not meant to be mutated after construction. Appending to or assigning into the metadata's own lists and dicts remains isolated, as before.🤖 Generated with Claude Code